Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ethereum,ethclient,eth/filters: marshal/unmarshal ethereum.FilterQuery struct to/from JSON correctly #23864

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ethereum,ethclient,eth/filters: marshal/unmarshal ethereum.FilterQuery struct to/from JSON correctly #23864

wants to merge 1 commit into from

Conversation

zenovich
Copy link
Contributor

@zenovich zenovich commented Nov 7, 2021 •
edited
Loading

  • Marshal ethereum.FilterQuery struct to JSON correctly,
  • Treat -1 as "latest" and -2 as "pending" (use rules from the rpc package),
  • Unmarshal ethereum.FilterQuery from JSON applying defaults for missing optional fields,
  • Use this implementation of marshalling in ethclient and eth/filters

fjl
fjl requested changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@fjl fjl left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot make this change because it is an API change. The FilterQuery struct cannot be modified.

If we want to add JSON marshaling for this type, we need to add MarshalJSON/UnmarshalJSON methods instead. The code for these methods already exists in other places in go-ethereum. For example, package ethclient contains a function that marshals FilterQuery for use with JSON-RPC.

@fjl fjl removed the status:triage label Nov 9, 2021
@zenovich
Copy link
Contributor Author

zenovich commented Nov 10, 2021 •
edited
Loading

No problem, @fjl. I'll rework that.

By the way, there is a question. The Ethereum Wiki (https://eth.wiki/json-rpc/API#parameters-45) says that if 'fromBlock' is missing it is treated as 'latest' while ethclient.toFilterArg() treats FromBlock==nil as 0x0 (i.e. "earliest") (see https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L406). Which path should I take?

(If you don't mind, I'd suggest not to reconstruct missing optional fields on marshalling, but apply default values in UnmarshalJSON() instead. This would probably make things easier.)

Also, there is an inconsistency between ethclient.toBlockNumArg() (https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L527) and the 'rpc' package constants (https://github.com/ethereum/go-ethereum/blob/master/rpc/types.go#L60): ethclient.toBlockNumArg() treats -1 as "pending" and -2 as -2 while the rpc package treats -1 as "latest" and -2 as "pending". Not sure which one is correct.

@fjl
Copy link
Contributor

fjl commented Nov 10, 2021

By the way, there is a question. The Ethereum Wiki (https://eth.wiki/json-rpc/API#parameters-45) says that if 'fromBlock' is missing it is treated as 'latest' while ethclient.toFilterArg() treats FromBlock==nil as 0x0 (i.e. "earliest") (see https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L406). Which path should I take?

It's documented in package ethereum that FromBlock == nil means genesis block, so we shouldn't change it.

Also, there is an inconsistency between ethclient.toBlockNumArg() and the 'rpc' package constants : ethclient.toBlockNumArg() treats -1 as "pending" and -2 as -2 while the rpc package treats -1 as "latest" and -2 as "pending". Not sure which one is correct.

This is unfortunate, and it's my fault. In the PR that introduced it (#21177), I suggested using -1 instead of allowing all negative numbers to mean 'pending'. I did not cross-check with package rpc at that time.

I'll think about how to fix this.

@zenovich zenovich changed the title ethereum: marshal FilterQuery struct to JSON correctly @zenovich ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct t… … f341664 …o JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters Nov 11, 2021
@zenovich zenovich changed the title @zenovich ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct t… … f341664 …o JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct t… … f341664 …o JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters Nov 11, 2021
@zenovich zenovich changed the title ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct t… … f341664 …o JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct to JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters Nov 11, 2021
@zenovich zenovich changed the title ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct to JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters ethereum,ethclient,eth/filters: marshal/unmarshal ethereum.FilterQuery struct to/from JSON correctly Nov 11, 2021
@zenovich
Copy link
Contributor Author

By the way, there is a question. The Ethereum Wiki (https://eth.wiki/json-rpc/API#parameters-45) says that if 'fromBlock' is missing it is treated as 'latest' while ethclient.toFilterArg() treats FromBlock==nil as 0x0 (i.e. "earliest") (see https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L406). Which path should I take?

It's documented in package ethereum that FromBlock == nil means genesis block, so we shouldn't change it.

Also, there is an inconsistency between ethclient.toBlockNumArg() and the 'rpc' package constants : ethclient.toBlockNumArg() treats -1 as "pending" and -2 as -2 while the rpc package treats -1 as "latest" and -2 as "pending". Not sure which one is correct.

This is unfortunate, and it's my fault. In the PR that introduced it (#21177), I suggested using -1 instead of allowing all negative numbers to mean 'pending'. I did not cross-check with package rpc at that time.

I'll think about how to fix this.

Fixed

@zenovich zenovich requested a review from fjl November 11, 2021 02:26
@zenovich
ethereum,ethclient,eth/filters: marshal ethereum.FilterQuery struct t…
6bddd1a 
…o JSON correctly, treat -1 as "latest" and -2 as "pending" (use rules from the rpc package), unmarshal it from JSON applying defaults for missing optional fields, reuse this marshalling implementation in ethclient and eth/filters
@fjl fjl self-assigned this Nov 17, 2021
@zenovich
Copy link
Contributor Author

zenovich commented Jan 8, 2022

@fjl, can we merge please?

@fjl
Copy link
Contributor

fjl commented Jan 27, 2022

Sorry this is taking so long. I have been reviewing this on and off between other tasks, and will change some things before we can get this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gballet gballet Awaiting requested review from gballet

@holiman holiman Awaiting requested review from holiman holiman is a code owner

@karalabe karalabe Awaiting requested review from karalabe karalabe is a code owner

@ligi ligi Awaiting requested review from ligi

@MariusVanDerWijden MariusVanDerWijden Awaiting requested review from MariusVanDerWijden

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

@fjl fjl Awaiting requested review from fjl

Requested changes must be addressed to merge this pull request.

Assignees

@fjl fjl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zenovich @fjl @ligi